Skip to content

Conversation

@riteshshukla04
Copy link
Contributor

@riteshshukla04 riteshshukla04 commented Mar 11, 2025

Summary:

Implemented Methods in URL which were not implemented yet
The methods output are identical to outputs of window.url .

Changelog:

[General][Added] URL accessors for unimplemented Methods

Test Plan:

Code:-
image

Tested with Hermes
image

Tested with JSC
image

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Mar 11, 2025
@riteshshukla04
Copy link
Contributor Author

riteshshukla04 commented Mar 11, 2025

@cipolleschi @cortinico Apologies for tagging . Can you check this Please. I know there are open PRs regarding this like #48333 . But all of them are waiting on feedbacks since months and are not in mergeable state as of now.
Please check if this can be merged .

@cortinico
Copy link
Contributor

@cipolleschi @cortinico Apologies for tagging . Can you check this Please. I know there are open PRs regarding this like #48333 . But all of them are waiting on feedbacks since months and are not in mergeable state as of now. Please check if this can be merged .

Can you rebase so that the CI can run on your PR?

@riteshshukla04
Copy link
Contributor Author

Done @cortinico . CIs are finally green

@riteshshukla04
Copy link
Contributor Author

@cortinico can you review this please

Copy link
Contributor

@cortinico cortinico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test case inside RNTester?

@riteshshukla04
Copy link
Contributor Author

@cortinico I have added a new URL example in rn tester. Can you check?

@facebook-github-bot
Copy link
Contributor

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@riteshshukla04
Copy link
Contributor Author

riteshshukla04 commented Mar 13, 2025

Oops lint failure :( . Are the lint rules different in meta tests? @cortinico can you pls help

@cortinico
Copy link
Contributor

cortinico commented Mar 13, 2025

Oops lint failure :( . Are the lint rules different in meta tests? @cortinico can you pls help

Yeah don't worry too much about the internal failures. Those are up to us to solve.

On the other hand: @rshest asked why is this change needed now. Like what's the use case?

@riteshshukla04
Copy link
Contributor Author

riteshshukla04 commented Mar 13, 2025

Alright @cortinico .
I understand that this change may not be super urgent but these URL methods were never implemented and needed to be implemented at some point.

This also helps toward react native's goal of moving towards more web standards
For Eg:- This is what happens when we call the url.password in console(web)
image

If the same code is used on latest RN(0.78.0) it will throw an error saying that this method was never implemented
image

It should also discourage React Native developers from using external libraries like urijs (which is deprecated in favor of the built-in URL API).

cc:- @rshest

@riteshshukla04
Copy link
Contributor Author

++
This also fixes issues like #45030 & #38966

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Mar 14, 2025
@facebook-github-bot
Copy link
Contributor

@cortinico merged this pull request in 3dac900.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @riteshshukla04 in 3dac900

When will my fix make it into a release? | How to file a pick request?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants